[SPARK-35005][SQL] Improve error msg if UTF8String concatWs length overflow #32106

ulysses-you · 2021-04-09T10:21:49Z

What changes were proposed in this pull request?

Add check if the byte length over int.

Why are the changes needed?

We encounter a very extreme case with expression concat_ws, and the error msg is

Caused by: java.lang.NegativeArraySizeException
	at org.apache.spark.unsafe.types.UTF8String.concatWs

Seems the UTF8String.concat has already done the length check at #21064, so it's better to add in concatWs.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

It's too heavy to add the test.

…erflow

SparkQA · 2021-04-09T11:20:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41707/

SparkQA · 2021-04-09T11:20:17Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41707/

SparkQA · 2021-04-09T13:07:13Z

Test build #137129 has finished for PR 32106 at commit fceb65b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2021-04-09T13:27:22Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

    // Allocate a new byte array, and copy the inputs one by one into it.
    // The size of the new array is the size of all inputs, plus the separators.
-    final byte[] result = new byte[numInputBytes + (numInputs - 1) * separator.numBytes];
+    int intNumInputBytes = Ints.checkedCast(numInputBytes + (numInputs - 1) * separator.numBytes);


Why did you use guava here instead of just checking numInputBytes > Int.MaxValue?

this code just follow the concat that do the same check.

spark/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

Line 974 in c067588

final byte[] result = new byte[Ints.checkedCast(totalLength)];

Could you use Math.toIntExact, and replace Ints.checkedCast by toIntExact, please. I don't see any benefits of the Guava function over the standard function, do you?

Replaced it.

SparkQA · 2021-04-12T02:13:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41766/

SparkQA · 2021-04-12T02:13:44Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41766/

SparkQA · 2021-04-12T03:11:04Z

Test build #137187 has finished for PR 32106 at commit 7411b6e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ulysses-you · 2021-04-12T04:35:09Z

retest this please

SparkQA · 2021-04-12T05:24:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41775/

SparkQA · 2021-04-12T05:24:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41775/

SparkQA · 2021-04-12T07:03:22Z

Test build #137196 has finished for PR 32106 at commit 7411b6e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2021-04-12T07:27:32Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+    int intNumInputBytes = Math.toIntExact(numInputBytes + (numInputs - 1) * separator.numBytes);
+    final byte[] result = new byte[intNumInputBytes];


If we want to prevent overflow, we should do that for all operators:

Suggested change

int intNumInputBytes = Math.toIntExact(numInputBytes + (numInputs - 1) * separator.numBytes);

final byte[] result = new byte[intNumInputBytes];

int resultSize = Math.toIntExact(Math.addExact(

numInputBytes,

Math.multiplyExact(numInputs - 1, separator.numBytes)));

final byte[] result = new byte[resultSize];

numInputBytes has changed to long. you mean (numInputs - 1) * separator.numBytes can overflow ? yeah, maybe it can be. How about this ?

int intNumInputBytes = Math.toIntExact( numInputBytes + (numInputs - 1) * (long)separator.numBytes);

you mean (numInputs - 1) * separator.numBytes can overflow ?

yep

How about this ?

I am ok with that.

MaxGekk · 2021-04-12T08:35:59Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

    // The size of the new array is the size of all inputs, plus the separators.
-    final byte[] result = new byte[numInputBytes + (numInputs - 1) * separator.numBytes];
+    int intNumInputBytes = Math.toIntExact(
+            numInputBytes + (numInputs - 1) * (long)separator.numBytes);


Could you re-check the indentation. I guess it should be smaller.

MaxGekk · 2021-04-12T08:36:33Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+    int intNumInputBytes = Math.toIntExact(
+            numInputBytes + (numInputs - 1) * (long)separator.numBytes);
+    final byte[] result = new byte[intNumInputBytes];


BTW, you can put this in one line:

Suggested change

int intNumInputBytes = Math.toIntExact(

numInputBytes + (numInputs - 1) * (long)separator.numBytes);

final byte[] result = new byte[intNumInputBytes];

int resultSize = Math.toIntExact(numInputBytes + (numInputs - 1) * (long)separator.numBytes);

final byte[] result = new byte[resultSize];

SparkQA · 2021-04-12T09:20:02Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41782/

SparkQA · 2021-04-12T09:28:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41784/

SparkQA · 2021-04-12T09:28:10Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41784/

SparkQA · 2021-04-12T11:05:42Z

Test build #137203 has finished for PR 32106 at commit 9081799.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-04-12T11:13:08Z

Test build #137206 has finished for PR 32106 at commit 73f4824.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2021-04-12T11:31:16Z

+1, LGTM. Merging to master.
Thank you @ulysses-you and @maropu for your review.

maropu · 2021-04-12T11:34:01Z

late lgtm

MaxGekk · 2021-04-12T11:38:28Z

@ulysses-you Would you like to review other places in UTF8String and make similar changes. For instance:

spark/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

Line 910 in 1be1012

byte[] data = new byte[this.numBytes + pad.numBytes * count + remain.numBytes];

ulysses-you · 2021-04-13T03:55:45Z

@MaxGekk created ticket SPARK-35041

ulysses-you · 2021-04-13T04:20:41Z

created #32142

[SPARK-35005][SQL] Improve error msg if UTF8String concatWs length ov…

fceb65b

…erflow

github-actions bot added the SQL label Apr 9, 2021

maropu reviewed Apr 9, 2021

View reviewed changes

use math

7411b6e

MaxGekk reviewed Apr 12, 2021

View reviewed changes

fix

9081799

MaxGekk reviewed Apr 12, 2021

View reviewed changes

inline

73f4824

MaxGekk approved these changes Apr 12, 2021

View reviewed changes

MaxGekk closed this in 1be1012 Apr 12, 2021

ulysses-you deleted the SPARK-35005 branch April 13, 2021 04:05

		int intNumInputBytes = Math.toIntExact(numInputBytes + (numInputs - 1) * separator.numBytes);
		final byte[] result = new byte[intNumInputBytes];

-    int intNumInputBytes = Math.toIntExact(numInputBytes + (numInputs - 1) * separator.numBytes);
-    final byte[] result = new byte[intNumInputBytes];
+    int resultSize = Math.toIntExact(Math.addExact(
+      numInputBytes,
+      Math.multiplyExact(numInputs - 1, separator.numBytes)));
+    final byte[] result = new byte[resultSize];

[SPARK-35005][SQL] Improve error msg if UTF8String concatWs length overflow #32106

[SPARK-35005][SQL] Improve error msg if UTF8String concatWs length overflow #32106

Uh oh!

Conversation

ulysses-you commented Apr 9, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Apr 9, 2021

Uh oh!

SparkQA commented Apr 9, 2021

Uh oh!

SparkQA commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

ulysses-you commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

MaxGekk Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

SparkQA commented Apr 12, 2021

Uh oh!

MaxGekk commented Apr 12, 2021

Uh oh!

maropu commented Apr 12, 2021

Uh oh!

MaxGekk commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses-you commented Apr 13, 2021

Uh oh!

ulysses-you commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MaxGekk Apr 12, 2021 •

edited

Loading

MaxGekk commented Apr 12, 2021 •

edited

Loading